DEV-793 Widen + always apply the slow-sensor CSV gap window - #285
DEV-793 Widen + always apply the slow-sensor CSV gap window#285marknolan wants to merge 6 commits into
Conversation
The DEV-927 hardware-validation recording (25 min of MLX90632 skin temp at 16 Hz output, 1506 blocks, no samples lost) fragmented into 7 CSVs. Two defects in the gap-window seeding: 1. The measured window never engaged: it is seeded from the first payload carrying >= 2 slow-sensor blocks, but when the recording's first payload holds only one block (refine returns early), the configured-rate +/-10% band from populateExpectedPayloadTsDiffLimitMapIfNeeded claims the global map key first and the refine path's containsKey guard then skips forever. The put is now unconditional - the measured window wins as soon as it exists and re-measures on every payload. 2. The gap side of the window was too tight even when measured: it came from the observed per-payload period spread, which for 2-3 blocks per payload is often a single gap (no spread information), while MLX90632 conversions can slip by several refresh periods and catch up (observed +12.5% block spacing). The gap side is now SLOW_SENSOR_MAX_INTER_BLOCK_GAP_RATIO (1.5x) of the achieved median spacing - healthy jitter stays continuous, a genuinely dropped block (2x spacing) still splits. The fast side keeps the observed-minimum- period basis with the standard tolerance. With both fixes the recording parses into the correct CSV sets: splits only at the genuine device events (a reset at recording start and an RWC re-sync during the download BLE connection). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adjusts how CSV-splitting gap windows are derived for slow sensors (VD6283 light, MLX90632 skin temp) so that normal conversion jitter and limited early payload information don’t cause false “data gap” splits.
Changes:
- Introduces a slow-sensor-specific maximum inter-block gap ratio constant (
SLOW_SENSOR_MAX_INTER_BLOCK_GAP_RATIO). - Updates slow-sensor gap-window seeding to use achieved median spacing for the “gap side” and to overwrite the global limits on every qualifying payload (instead of being locked by an early configured-rate seed).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ShimmerDriver/src/main/java/com/shimmerresearch/verisense/payloaddesign/UtilCsvSplitting.java | Adds a slow-sensor-specific gap-ratio constant used when building splitting limits. |
| ShimmerDriver/src/main/java/com/shimmerresearch/verisense/payloaddesign/PayloadContentsDetailsV8orAbove.java | Changes slow-sensor rate refinement to widen the gap-side window and always apply measured limits per payload. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…review) The first paragraph still described the pre-fix containsKey-guarded / first-payload seeding, contradicting the unconditional-put explanation below it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jyong15
left a comment
jyong15
left a comment
There was a problem hiding this comment.
Code review — DEV-793 slow-sensor CSV gap window
Recommended disposition: request changes. The diagnosis of the original bug is correct — I verified that populateExpectedPayloadTsDiffLimitMapIfNeeded (UtilCsvSplitting.java:85) runs after refineSlowSensorSamplingRateFromBlockTicks in parsePayloadContentsMetaData, so a first payload carrying <2 slow blocks did permanently lock in the configured-rate ±10% band. But dropping the containsKey guard trades that bug for a dropped-block blind spot.
Major
1. The gap window is now self-referential, so a dropped slow-sensor block goes undetected — and the split lands at the wrong boundary instead (PayloadContentsDetailsV8orAbove.java:399-407). With the guard removed, the window is re-derived every payload from the very block spacings it then judges — and the two quantities are identically the same expression: refine computes perSamplePeriodsS[i] = (deltaTicks/32768.0)/sampleCount, and the gap check computes calcSamplingRate(prevEnd, nextEnd, nextSampleCount) = sampleCount/Δt(s). So 1/measuredPeriod is exactly the rate the checker computes for that boundary, and it is always inside [rate/1.5, rate*1.1].
Failure scenario with true per-sample period P and a payload carrying exactly 2 skin-temp blocks (2–3 per payload is the norm per the PR body) with one block's worth of data dropped between them (spacing 2P):
| value | |
|---|---|
perSamplePeriodsS |
[2P] → medianPeriodS = 2P, achievedRateHz = 0.5/P |
| new limits | [0.333/P, 0.55/P] |
intra-payload boundary rate 0.5/P |
inside → real gap judged continuous, silently absorbed |
| both blocks | setSamplingRate(0.5/P) → that payload's CSV timestamps stretched 2× |
previous-payload boundary (healthy 1/P) |
> 0.55/P → flagged → CSV splits one block early, at a boundary with no gap |
Pre-PR, the seeded fixed window (e.g. [0.667/P, 1.1/P]) correctly split at the real gap. Suggested direction: keep re-measuring, but don't let one payload's measurement replace the window wholesale — e.g. only accept a measurement from a payload with ≥3 observed periods, or carry a running/first-robust estimate forward and only widen it, rather than puting the latest value unconditionally.
2. The fast side is now volatile too, though the PR describes it as unchanged (:402). The two sides have inconsistent bases: gap side = median (robust), fast side = 1.1/min(observedPeriods) — a single-sample extremum, now re-sampled every payload. Too tight: the scenario above gives limits[1] = 0.55/P, rejecting the true healthy rate. Too loose: one catch-up gap of 0.5P (the PR body's own "slip then catch up" mechanism) gives limits[1] = 2.2/P, effectively disabling overlap/fast detection for that payload's intra- and cross-payload checks. Consider achievedRateHz * UPPER or a symmetric ratio constant, so both sides derive from the median.
Minor
3. perSamplePeriodsS.get(size/2) is an upper-median (:366) — on a 2-element sorted list it returns the larger value, so for a 3-block payload with one dropped block ([P, 2P]), the "median" is 2P and achievedRateHz is half the truth, contradicting the comment on line 364. That value now feeds both setSamplingRate() and limits[0], and the resulting window [0.333/P, 1.1/P] contains both boundary rates, so the gap is missed entirely and all three blocks are stamped at half rate. Pre-existing line, but the new formula gives it a much larger role — worth fixing here (even size → average the two middles).
4. The new constant's javadoc is stale and self-contradictory (UtilCsvSplitting.java:24-28): "the window is seeded from the first payload that carries >= 2 blocks" is precisely the behavior this PR removes (commit 2 fixed the equivalent wording in PayloadContentsDetailsV8orAbove.java but not here), and "1.5x keeps comfortable margin on both sides" is wrong — the ratio applies only to the gap side; the fast side uses UPPER.
5. No driver-side unit test for the new arithmetic. Nothing in ShimmerDriver/src/test touches UtilCsvSplitting, SAMPLING_RATE_LIMITS_PER_SENSOR, or refineSlowSensorSamplingRateFromBlockTicks; validation is entirely ASM_PC integration tests in a separate, currently-blocked PR. The window computation is a pure function of a List<Double> of periods — the scenarios in findings 1 and 3 would each be a few-line unit test and would have caught them. Note the passing Test_065 recording is described as having no lost samples, so it never exercises the dropped-block path.
Nits
6. ShimmerDriver/build.gradle:52 stays 0.11.8_beta — if the Jenkins publish ASM_PC is blocked on reuses those coordinates, consumers can resolve a stale cached artifact of the same version. Flagging only so the uprev sequencing is explicit.
7. public class FILE_GAP_TOLERANCE_MULTIPLIER (UtilCsvSplitting.java:14) should be static — a non-static inner class holding only constants. Pre-existing; this PR adds a third constant to it.
Checked and cleared
- No public/protected signature changes; the new constant is additive.
SAMPLING_RATE_LIMITS_PER_SENSORis only written internally, so the removed guard can only be overriding the configured-rate fallback, as claimed. - No cross-file leakage: ASM_PC calls
clearMapOfSamplingRateLimitsPerSensor()on every CSV-set boundary. - No new threading race within this repo (no parallel payload parsing) — though the unsynchronised static
HashMapremains one if a consumer ever parses two files concurrently in one JVM. - Unit consistency of
achievedRateHz/1.5holds whenever the median is measured correctly. maxPeriodSremoval leaves no dead code;FILE_GAP_TOLERANCE_MULTIPLIER.LOWERis still used bycalculateSamplingRateLimits.
Findings 1 and 3 diminish if real payloads typically carry ≥4 slow-sensor blocks — the 2–3 figure is taken from the PR description, not measured.
Generated by Claude Code
…n + symmetric limits Review follow-up for #285. The window that decides whether a slow-sensor (VD6283 light / MLX90632 skin temp) block boundary is continuous was re-derived on every payload from the same handful of inter-block spacings it then had to judge, and its fast edge came from a single extremum. - Accumulate the measured per-sample periods per sensor across the payloads of a parse run (UtilCsvSplitting.SLOW_SENSOR_OBSERVED_PERIODS_PER_SENSOR) instead of replacing the estimate with the latest payload's values. A payload only carries 2-3 slow-sensor blocks, so a per-payload window absorbed a dropped block's 2x spacing into its own centre and never reported it - while flagging the healthy boundary back to the previous payload instead. Against a history spanning hundreds of payloads a single 2x outlier barely moves the median. The history is bounded (SLOW_SENSOR_PERIOD_HISTORY_MAX, oldest dropped first) so it still follows genuine long-term drift, and it shares its lifecycle with SAMPLING_RATE_LIMITS_PER_SENSOR - clearMapOfSamplingRateLimitsPerSensor() now clears both, so measurements cannot leak across a CSV-set boundary. - Derive BOTH sides of the window from that median: limits[0] = median / SLOW_SENSOR_MAX_INTER_BLOCK_GAP_RATIO, limits[1] = median * FILE_GAP_TOLERANCE_MULTIPLIER.UPPER. The fast side previously used 1/minObservedPeriod, i.e. a single-sample extremum. - Median helper now averages the two middle values for an even-sized input (the old get(size/2) was the upper median) and sorts a copy. The data blocks' sampling rate is set from the accumulated median too, so the CSV timestamps and the continuity check agree on the achieved cadence. - Rewrote the stale/incorrect SLOW_SENSOR_MAX_INTER_BLOCK_GAP_RATIO javadoc: it no longer claims first-payload seeding (removed by #285) and no longer claims the ratio applies to both sides. - FILE_GAP_TOLERANCE_MULTIPLIER is now a static nested class. - New driver-side JUnit tests (API_00009_UtilCsvSplittingSlowSensorGapWindow, 12 cases, no hardware data needed) covering the even-count median, +12.5% healthy jitter staying continuous, a dropped block being detected even though its own payload fed the estimate, the measured window taking over from a fallback-seeded band (and the fallback not clobbering it back), history bounding and the clear contract. The unconditional put is preserved: the measured window must still win over the configured-rate +/-10% band that populateExpectedPayloadTsDiffLimitMapIfNeeded seeds, as soon as a measurement exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014EmM3GD2F6zPqXmWHS94dh
remove(0) in a loop shifts the whole ArrayList once per removed element; clearing the excess head range via subList does it in a single pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014EmM3GD2F6zPqXmWHS94dh
…r-prs-bm3j94 DEV-793 Robust slow-sensor gap window: accumulated median + symmetric limits (review follow-up for #285)
| int excess = accumulatedPeriodsS.size()-SLOW_SENSOR_PERIOD_HISTORY_MAX; | ||
| if(excess>0) { | ||
| accumulatedPeriodsS.subList(0, excess).clear(); | ||
| } | ||
| return calculateMedian(accumulatedPeriodsS); |
| // The header-derived 10 Hz is an estimate; the achieved cadence is ~9.09 Hz, | ||
| // which the +/-10% band already calls a gap on every single boundary (this | ||
| // fragmented a 25-min DEV-927 recording into 7 CSVs). | ||
| assertTrue("baseline: the fallback band is too tight for the achieved cadence", | ||
| UtilCsvSplitting.isSamplingRateOutsideOfLimits(currentLimits(), boundaryRateHz(NOMINAL_PERIOD_S*1.125))); |
Problem
The DEV-927 hardware-validation recording (25 min of MLX90632 skin temp at 16 Hz output — 1,506 blocks, tick-verified no samples lost) fragmented into 7 skin-temp CSVs when parsed. Healthy MLX conversion jitter (block spacing occasionally +12.5%, then catching up) was being treated as a data gap.
Two defects, both in the slow-sensor gap-window seeding
refineSlowSensorSamplingRateFromBlockTicksreturns early andpopulateExpectedPayloadTsDiffLimitMapIfNeededclaims the global map key with a configured-rate ±10% band. The refine path'scontainsKeyguard then skips forever. The put is now unconditional: the measured window wins as soon as it exists, and re-measures each payload.SLOW_SENSOR_MAX_INTER_BLOCK_GAP_RATIO(1.5×) of the achieved median spacing: healthy jitter stays continuous, while a genuinely dropped block (2× spacing) still splits with comfortable margin. The fast side keeps the observed-minimum-period basis with the standard ±10%.Result
The recording parses into the correct CSV sets — splits only at genuine device events (a reset at recording start; an RWC re-sync during the download BLE connection, uptime vs wall-clock verified).
Validation
ASM_PC
ASM_PC_00005_VerisenseFileParserPC72/72 green (including the new Test_065 built from this recording) +ASM_PC_000327/7, run against this branch. All pre-existing tests byte-identical — the widened window changes no previously-passing output (it only removes spurious splits).Sequencing
ASM_PC PR (Test_065 + UTF-8 writers) is blocked on a Jenkins
shimmerdriverpublish containing this — same flow as the recent 0.11.8_beta.Jira: DEV-793 / DEV-927
🤖 Generated with Claude Code